Skip to content

feat(room-worker): actor-named, count-based membership system messages#419

Open
vjauhari-work wants to merge 1 commit into
mainfrom
claude/system-message-room-members-e96h03
Open

feat(room-worker): actor-named, count-based membership system messages#419
vjauhari-work wants to merge 1 commit into
mainfrom
claude/system-message-room-members-e96h03

Conversation

@vjauhari-work

@vjauhari-work vjauhari-work commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator

Summary

Membership system messages (create-channel / add-members / remove-member) are now actor-named and count-based so the frontend can render clickable lists of who was added/removed.

  • members_added (add + create): "Alice" added 2 people and 1 organization to the chatroom, built by a single shared addedContent helper used by both add-members and create-channel. A lone individual (no orgs) still renders the named form ("Alice" added "Bob" to the chatroom).
    • Payload lists come from the resolved member set — req.Users for add, req.ResolvedUsers for create — so channel-expanded individuals are included and org members are represented as orgs, never as people.
    • An empty channel emits no members_added (only room_created).
    • individuals / orgs always marshal as [] (never null).
  • member_removed / member_left: removals are actor-named ("Alice" removed "Bob" from the chatroom); wording changed from "channel" to "chatroom" across all membership messages.
  • docs/client-api.md: documents the members_added / member_removed / member_left sysMsgData payload schema (the clickable-list wire contract).
  • history-service: integration test asserting sysMsgData survives the load-history round-trip (verified against real Cassandra).

No pkg/model struct changes, no room-service / materialization changes, and no new database reads.

Test Plan

  • make lint — 0 issues
  • make test — unit tests green (whole repo)
  • make test-integration SERVICE=room-worker — pass (real Mongo)
  • make test-integration SERVICE=history-service — pass (real Cassandra; sysMsgData round-trip)
  • Verified the Load History RPC returns sysMsgData for system messages (read scans it; LoadHistory passes it through untouched)

🤖 Generated with Claude Code

https://claude.ai/code/session_01WuJMTQH2mnUqKd6jhSg8HK


Generated by Claude Code

Summary by CodeRabbit

  • Documentation

    • Expanded client API docs for system-message sysMsgData, adding type-dependent JSON payload shapes for member-added/removed/left and how to decode ClientMessage.sysMsgData.
  • Bug Fixes

    • Updated rendered system-message text for member/org removals and “members_added” to use requester-aware wording, consistent “chatroom” terminology, and count-based phrasing.
    • Refined members_added payload/content to reflect requester-excluded resolved member/org sets and ensure empty slices serialize as [] and that irrelevant “added” messages are skipped.
  • Tests

    • Updated/added unit and integration tests to validate sys_msg_data round-tripping and the new rendered/payload expectations.

@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@vjauhari-work, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 52 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5a9a4818-9d98-4e53-95af-376fc34b386e

📥 Commits

Reviewing files that changed from the base of the PR and between 9d5f44a and 089f467.

📒 Files selected for processing (9)
  • docs/client-api.md
  • history-service/internal/cassrepo/messages_by_room_integration_test.go
  • pkg/model/member.go
  • room-service/handler.go
  • room-worker/handler.go
  • room-worker/handler_test.go
  • room-worker/integration_test.go
  • room-worker/sysmsg.go
  • room-worker/sysmsg_test.go
📝 Walkthrough

Walkthrough

Sys-message rendering now uses chatroom wording, requester-aware removal text, and count-based add messages. Handler paths, persistence coverage, and client docs were updated for typed sysMsgData payloads and the new members_added shape.

Changes

Sys-message formatting and payload updates

Layer / File(s) Summary
sysmsg.go helpers
room-worker/sysmsg.go
Introduces pluralized count formatting, single-person add formatting, lookup-based added content selection, non-nil slice normalization, and requester-aware remove/leave message helpers with chatroom wording.
handler.go call sites
room-worker/handler.go
Passes requester context into removal formatting, changes processAddMembers to build members_added payloads from request lists, and updates publishChannelSysMessages to use resolved member sets and skip empty additions.
Tests for sys-message shapes
room-worker/sysmsg_test.go, room-worker/handler_test.go
Updates helper tests for chatroom wording, pluralization, lookup-driven content, and fallback names, and expands handler tests to assert the new members_added and removal message shapes.
Integration, persistence, and docs
room-worker/integration_test.go, history-service/internal/cassrepo/messages_by_room_integration_test.go, docs/client-api.md, pkg/model/member.go, room-service/handler.go
Updates room-worker integration expectations, adds Cassandra round-trip coverage for typed sys_msg_data payloads, and documents the payload contract in the client API and model comments.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

  • hmchangw/chat#185: Overlaps with the same room-worker sys-message formatting and requester-aware removal/addition paths.
  • hmchangw/chat#142: Shares the members_added emission flow and the create-room / publish-system-message path.
  • hmchangw/chat#84: Connects through the typed members_added payload shape and persistence of sysMsgData.

Suggested labels: ready

Suggested reviewers: mliu33

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 48.84% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and accurately captures the main room-worker change: actor-named, count-based membership system messages.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/system-message-room-members-e96h03

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/client-api.md`:
- Around line 2335-2356: The docs for sysMsgData are incomplete and should be
updated to match the runtime payloads exposed by model.MembersAdded and the
member_* shapes. Expand the members_added table to include the missing channels
([ChannelRef][]) and addedUsersCount fields, and add a shared named SysMsgUser
schema for the user object so member_removed and member_left reference it
instead of an inline compound type. Also add JSON examples for members_added,
member_removed, and member_left so clients can parse each payload shape
correctly.

In `@history-service/internal/cassrepo/messages_by_room_integration_test.go`:
- Around line 351-373: The round-trip test for `GetMessagesBefore` only covers
populated `MembersAdded` slices, so it does not verify the empty-array
serialization behavior. Extend `messages_by_room_integration_test.go` by adding
a case around `model.MembersAdded` that uses an empty `Individuals` or `Orgs`
slice, then load it through `repo.GetMessagesBefore` and assert the decoded
slice is non-nil and has length 0. Keep the existing
`model.MessageTypeMembersAdded` and `json.Unmarshal` checks so the test locks
down the schema change for empty collections too.

In `@room-worker/handler_test.go`:
- Around line 4173-4193: The test for publishChannelSysMessages currently only
checks that MessageTypeMembersAdded is not emitted, so it can still pass if
nothing is published at all. Update
TestHandler_PublishChannelSysMessages_EmptySkipsMembersAdded to also assert that
a room_created event is emitted by publishChannelSysMessages for the create-room
path, using the existing published slice and model.MessageEvent checks alongside
the current members_added negative assertion.
- Around line 4116-4171: The tests for publishChannelSysMessages only verify the
populated side of model.MembersAdded and miss the empty-slice contract, so add
assertions in the existing create-room cases to check that users-only publishes
a non-nil empty Orgs slice and orgs-only publishes a non-nil empty Individuals
slice. Use the existing TestHandler_PublishChannelSysMessages_UsersOnly and
TestHandler_PublishChannelSysMessages_OrgsOnly cases, and validate the
unmarshaled payload fields in the same spot where payload.Individuals is already
checked, so regressions to null or omitted empty collections are caught.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a5ff251b-328f-4856-b453-7bc9f158e7c7

📥 Commits

Reviewing files that changed from the base of the PR and between d2931f0 and 78a63c8.

📒 Files selected for processing (8)
  • docs/client-api.md
  • history-service/internal/cassrepo/messages_by_room_integration_test.go
  • pkg/model/member.go
  • room-worker/handler.go
  • room-worker/handler_test.go
  • room-worker/integration_test.go
  • room-worker/sysmsg.go
  • room-worker/sysmsg_test.go

Comment thread docs/client-api.md
Comment thread room-worker/handler_test.go
Comment thread room-worker/handler_test.go
@vjauhari-work vjauhari-work force-pushed the claude/system-message-room-members-e96h03 branch from 78a63c8 to 8033878 Compare June 29, 2026 10:53

Copy link
Copy Markdown
Collaborator Author

Addressed CodeRabbit's review (force-pushed as 8033878):

  • docs/client-api.mdmembers_added table now documents channels ([ChannelRef](#channelref)[]) and addedUsersCount; the user field references a new named SysMsgUser schema instead of an inline object; added JSON examples for members_added / member_removed / member_left.
  • history-service test — added TestGetMessagesBefore_SysMsgDataEmptySlice asserting an empty individuals survives as [] (non-nil, len 0), not null.
  • handler_test.go — the create users-only/orgs-only tests now assert the opposite slice is non-nil empty ([]-vs-null contract); ..._EmptySkipsMembersAdded now also asserts room_created is published (so it can't pass vacuously).

Not applied: the Docstring Coverage pre-merge warning (47.62% < 80%). This repo's conventions (CLAUDE.md §3) mandate minimal comments (≤2 lines, why-not-what); adding boilerplate docstrings to satisfy the threshold would conflict with that, and it's an advisory check, not a blocking gate.

Verified: make lint clean, make test green, and the room-worker + history-service integration suites pass against real Mongo/Cassandra.


Generated by Claude Code

@vjauhari-work vjauhari-work force-pushed the claude/system-message-room-members-e96h03 branch 3 times, most recently from c665713 to 9d5f44a Compare July 2, 2026 03:46

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
room-worker/handler_test.go (1)

4032-4306: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider a shared test-setup helper to cut boilerplate.

Nearly every new test in this range (Lines 4032-4306) repeats the same gomock.NewController + NewMockSubscriptionStore + published []publishedMsg + &Handler{...} construction. A helper mirroring newCreateRoomTestHandler (used at Line 2333) for the add/remove-member tests would cut duplication and make future additions less error-prone.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@room-worker/handler_test.go` around lines 4032 - 4306, The add-member and
channel sys-message tests repeat the same controller/store/published-handler
setup, so extract that boilerplate into a shared helper like the existing
create-room test helper. Add a helper near these tests that builds the gomock
controller, NewMockSubscriptionStore, published slice, and Handler with the
common siteID/keyStore/keySender wiring, then update the affected tests to use
it so future add/remove-member cases stay consistent and easier to maintain.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@room-worker/handler_test.go`:
- Around line 4107-4156: The add-members sys-msg test in
TestHandler_ProcessAddMembers_RequesterExcludedFromSysMsg should also verify the
empty Orgs contract. Extend the payload assertions after unmarshalling
model.MembersAdded to check that payload.Orgs is non-nil and empty, matching the
same nonNil serialization behavior already covered in the
PublishChannelSysMessages tests. This keeps the processAddMembers path aligned
with the existing members_added payload contract alongside Individuals and
Channels.
- Around line 4252-4275: The test in
TestHandler_PublishChannelSysMessages_OrgsOnly hard-codes the added users count,
which leaves the emitted sysmsg payload field unverified. Replace the fixed
value passed into publishChannelSysMessages with a scenario-derived count, or
add an assertion on payload.AddedUsersCount after unmarshalling the MembersAdded
payload, so the test checks the actual value written by
Handler.publishChannelSysMessages.

---

Nitpick comments:
In `@room-worker/handler_test.go`:
- Around line 4032-4306: The add-member and channel sys-message tests repeat the
same controller/store/published-handler setup, so extract that boilerplate into
a shared helper like the existing create-room test helper. Add a helper near
these tests that builds the gomock controller, NewMockSubscriptionStore,
published slice, and Handler with the common siteID/keyStore/keySender wiring,
then update the affected tests to use it so future add/remove-member cases stay
consistent and easier to maintain.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ae734fa2-207a-47d2-91fc-3929229499a3

📥 Commits

Reviewing files that changed from the base of the PR and between c665713 and 9d5f44a.

📒 Files selected for processing (9)
  • docs/client-api.md
  • history-service/internal/cassrepo/messages_by_room_integration_test.go
  • pkg/model/member.go
  • room-service/handler.go
  • room-worker/handler.go
  • room-worker/handler_test.go
  • room-worker/integration_test.go
  • room-worker/sysmsg.go
  • room-worker/sysmsg_test.go
✅ Files skipped from review due to trivial changes (3)
  • room-service/handler.go
  • pkg/model/member.go
  • room-worker/integration_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
  • history-service/internal/cassrepo/messages_by_room_integration_test.go
  • docs/client-api.md
  • room-worker/sysmsg.go
  • room-worker/handler.go
  • room-worker/sysmsg_test.go

Comment thread room-worker/handler_test.go
Comment thread room-worker/handler_test.go
Make create-channel / add-members / remove-member system messages
actor-named and count-based so the frontend can render clickable lists of
who was added/removed:

- members_added (add + create): "Alice added 2 people and 1 organization to
  the chatroom", with a single shared addedContent helper used by both flows;
  a lone individual (no orgs) still renders the named form. Payload lists are
  the resolved member set — req.Users for add, req.ResolvedUsers for create
  (channel-expanded individuals included, org members represented as orgs).
  An empty channel emits no members_added (room_created still fires).
- member_removed / member_left: actor-named removals
  ("Alice removed Bob from the chatroom"); wording "channel" -> "chatroom"
  across all membership messages.
- Empty individuals/orgs lists marshal as [] (not null) for the frontend.
- docs/client-api.md: document the members_added / member_removed /
  member_left sysMsgData payload schema.
- history-service: integration test asserting sysMsgData survives the
  load-history round-trip.

No pkg/model struct changes, no room-service/materialization changes, and no
new database reads.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WuJMTQH2mnUqKd6jhSg8HK
@vjauhari-work vjauhari-work force-pushed the claude/system-message-room-members-e96h03 branch from 9d5f44a to 089f467 Compare July 2, 2026 03:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants